Skip to content

feat(material-experimental/theming): Introduce a facade layer between user-facing customizable keys and actual MDC token names #27219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 9, 2023

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Jun 2, 2023

This allows us to expose easier to understand names for users, and
decouples us from changes that MDC might make to token names in the
future

@mmalerba mmalerba added the target: patch This PR is targeted for the next patch release label Jun 2, 2023
@mmalerba mmalerba requested a review from devversion as a code owner June 2, 2023 00:03
@mmalerba mmalerba requested a review from andrewseguin as a code owner June 2, 2023 00:03
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jun 2, 2023
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an example somewhere of how the API for customizing the tokens can be used?

@mmalerba mmalerba added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Jun 4, 2023
@mmalerba
Copy link
Contributor Author

mmalerba commented Jun 4, 2023

@crisbeto the second commit here does show an example of customizing tokens, are you asking for an example that's more setting one-off toke values rather than just theme-type or color-palette? I can certainly add that as well, might be good to show

@crisbeto
Copy link
Member

crisbeto commented Jun 4, 2023

Yeah I was thinking of a demo showing how a user can override an arbitrary token (e.g. the border radius of something).

@github-actions
Copy link

github-actions bot commented Jun 4, 2023

Deployed dev-app for fabab6e to: https://ng-dev-previews-comp--pr-angular-components-27219-pj0g4m6m.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

@devversion devversion removed their request for review June 5, 2023 11:31
mmalerba added 3 commits June 5, 2023 17:13
user-facing customizable keys and actual MDC token names

This allows us to expose easier to understand names for users, and
decouples us from changes that MDC might make to token names in the
future
@mmalerba
Copy link
Contributor Author

mmalerba commented Jun 5, 2023

Added an example of setting arbitrary custom token values

@crisbeto
Copy link
Member

crisbeto commented Jun 5, 2023

Is the example supposed to work in the dev app or is this just to show the API?

Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna make a suggestion that I think might help carve out this design, but don't hold yourself to it:

Write the user-facing documentation while you are creating these early designs and prototypes.

This would help us understand from a user-point-of-view whether there's anything that we might be missing or isn't intuitive, and it would help as we review as well.

@mmalerba
Copy link
Contributor Author

mmalerba commented Jun 5, 2023

@andrewseguin I don't want to spend too much time wordsmithing if its likely to change, but I can write up a rough bullet point version of the user docs, I think that would be helpful 👍

@mmalerba
Copy link
Contributor Author

mmalerba commented Jun 6, 2023

@crisbeto the example should work, you just need the magic param to turn on experimental styling: https://ng-dev-previews-comp--pr-angular-components-27219-pj0g4m6m.web.app/checkbox?tokenapi= (note: all of the other components will be un-styled in this mode)

Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved - if there's any changes we want to make over time we can do that iteratively, but this seems good to land now

@mmalerba
Copy link
Contributor Author

mmalerba commented Jun 8, 2023

Yep, nothing is set in stone here, that's why its in material-experimental. I do expect some of it to change and that's fine

@mmalerba mmalerba added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jun 8, 2023
@mmalerba mmalerba merged commit 227a741 into angular:main Jun 9, 2023
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker detected: feature PR contains a feature commit dev-app preview When applied, previews of the dev-app are deployed to Firebase target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants